Extended error handling when a node fails#64
Conversation
…models/common.py and core/models/last_run.py
… into feat/extended-error-handling
… into feat/extended-error-handling
…lass + a few unit tests
…o the RunError generated in run_node in run_job.py
… into feat/extended-error-handling
| description="Error message to use", | ||
| ) | ||
| error_type: str = Field( | ||
| default="ValueError", |
There was a problem hiding this comment.
maybe we could use exception type instead of string here and then when we check we dont need to compare strings
There was a problem hiding this comment.
I think it is useful to be able to insert a string here - in particular for the manual testing when running this node via the frontend.
| Scope is explicitly set to "function" to ensure each test gets fresh | ||
| node instances, maintaining test independence. | ||
| """ | ||
| from qualibrate import QualibrationLibrary |
There was a problem hiding this comment.
is there an advantage putting it locally in the fixture?
we might use it in the future in more fixtures so maybe we should change the locations of the local imports to the whole file
There was a problem hiding this comment.
You're right - simplifying it now, putting imports at the top of the file.
|
|
||
|
|
||
| @pytest.fixture(scope="function") | ||
| def test_library() -> Any: |
There was a problem hiding this comment.
Why is it Any? don't we return QualibrationLibrary?
in general typing needs to be added in code wherever we can avoid using Any.
There was a problem hiding this comment.
You're right! Fixing it.
| """Integration tests for simple nodes without action system.""" | ||
|
|
||
| def test_simple_node_success( | ||
| self, test_library: Any, fresh_state: Any |
There was a problem hiding this comment.
You're right! Fixing it.
| self, test_library: Any, fresh_state: Any | ||
| ) -> None: | ||
| """Test successful execution of simple node.""" | ||
| node = test_library.nodes["node_raises_in_body"] |
There was a problem hiding this comment.
maybe we should change the name of the node because from the name it sounds like this node should raise an exception from body, but we actually test the node runs successfully
There was a problem hiding this comment.
I changed the name to node_can_raise_in_body
| from qualang_tools.results import progress_counter | ||
| from qualang_tools.units import unit | ||
| from qualibrate import NodeParameters, QualibrationNode | ||
| from qualibration_libs.data import XarrayDataFetcher |
There was a problem hiding this comment.
if we remove dependency in qualibration_libs this should be taken in count aswell i assume
| assert last_run.name == long_name | ||
| assert len(last_run.name) > 300 | ||
|
|
||
| def test_many_state_updates(self, aware_datetime: Any) -> None: |
There was a problem hiding this comment.
You're right! I deleted the test.
|
|
||
| assert len(last_run.state_updates) == 50 | ||
|
|
||
| def test_complex_passed_parameters(self, aware_datetime: Any) -> None: |
There was a problem hiding this comment.
You're right! I deleted the test.
| assert fresh_state.last_run.error is None | ||
|
|
||
| @patch("qualibrate_runner.core.run_job.get_active_library_or_error") | ||
| def test_captures_workflow_snapshot_idx( |
There was a problem hiding this comment.
Do we need this tests after all the previous ones? if i understood right we checked it in the previous tests
There was a problem hiding this comment.
You're right - I deleted the test.
| freezegun = "^1.2.0" | ||
| xarray = "^2024.1.0" | ||
| quam-builder = {git = "https://github.com/qua-platform/quam-builder.git"} | ||
| #qualibration-libs = {path = "../qualibration-libs", develop = true} |
There was a problem hiding this comment.
we should remove dependency of qualibration-libs
… into feat/extended-error-handling
… into feat/extended-error-handling
Part of solving https://quantum-machines.atlassian.net/browse/QUAL-1645
Enhances error reporting by extending the RunError model with optional details_headline and details fields. When nodes raise errors, these fields propagate structured error information for better debugging and frontend display.
A major part of the work here is adding comprehensive testing, including node end-to-end tests. The tests depends on qualibrate-core, so this PR will have failing tests until the qua-platform/qualibrate-core#143 PR in qualibrate-core has been merged!
Key Changes
Test Coverage